Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update MacOS Runner #6580

Merged
merged 87 commits into from
Jul 21, 2024
Merged

Update MacOS Runner #6580

merged 87 commits into from
Jul 21, 2024

Conversation

CoolSpy3
Copy link
Contributor

@CoolSpy3 CoolSpy3 commented Jul 10, 2024 •
edited
Loading

Description
GitHub recently deprecated the macos-11 runner, so all of the tests which rely on that os are failing. This PR updates the runner to the latest macos-14 macos-13 image. (There's some bug installing cppcheck on macos-14. I might play around with bumping the cppcheck version later, but I don't have enough info to resolve it as-is.)

Hopefully, most of the workflow can be ported without too much hassle, however, if that proves to not be the case, I may have to leave this fix for someone else, as I do not have a mac to debug on.

@CoolSpy3 CoolSpy3 added test suite issue Test failure in the test suite CI test distribution Start the distribution test test suite Start the test suite test sources Start the sources test on all platforms test webots build Start the build tests labels Jul 10, 2024
@CoolSpy3 CoolSpy3 mentioned this pull request Jul 10, 2024
Merged
4 tasks
@CoolSpy3
Copy link
Contributor Author

The build passes! I'll come back tomorrow and see if I can get cppcheck working. If not, I'll just revert to this commit.

SIde note: Even though the build runs on macos 13, it still targets macos 11. I assume that we want to keep that as-is because we do not want to drop old support unless it becomes necessary.

@omichel
Copy link
Member

omichel commented Jul 10, 2024

No, the plan is to support only the maintained versions macOS, that are currently 12 (Monterey), 13 (Ventura) and 14 (Sonoma). So we should drop macOS 11 Big Sur and update the requirements documentation accordingly.

CoolSpy3 and others added 20 commits July 18, 2024 00:56
@CoolSpy3
revert last two commits
603a964
@CoolSpy3
try installing cppcheck as admin
0d5c258
@CoolSpy3
upload msiexec log
2ef8378
@CoolSpy3
Revert "upload msiexec log"
2b680be 
This reverts commit 2ef8378.
@CoolSpy3
install msi with powershell
bb83e19
@CoolSpy3
fix quotation marks
2a3a639
@CoolSpy3
use -ArgumentList
863cce8
@CoolSpy3
fix constVariablePointer warnings (round 2)
0c3d9de
@CoolSpy3
fix constParameterPointer warnings (round 2)
d746f82
@CoolSpy3
fix other warnings (round 2)
56694c5
@CoolSpy3
fix build errors
b5714f7
@CoolSpy3
fix more warnings
e077ee9
@CoolSpy3
run clang-format
f3060fe
@CoolSpy3
run clang-format
bd3202a
@CoolSpy3
run clang-format on c code
846a66a
@CoolSpy3
run clang-format@14
28e7cf6
@CoolSpy3
fix cppcheck warnings on Windows
acbf0eb
@CoolSpy3
fix windows build
92dee8b
@CoolSpy3
store bool values before calling assert
75346d9 
In previous commits, I wrapped a few calls in asserts. It looks like these were being optimized out in the release build.
@CoolSpy3
don't use unnecessary variables
fc7b143
@CoolSpy3 CoolSpy3 marked this pull request as ready for review July 21, 2024 00:26
@CoolSpy3 CoolSpy3 requested a review from a team as a code owner July 21, 2024 00:26
@CoolSpy3
Copy link
Contributor Author

We've done it! This PR is ready for review!
A summary of the changes:

  • Updated the macos runner to version 14
    • Once this PR is merged, I can migrate this change to the wiki
  • Removed support for macos 11
  • Bumped cppcheck to version 2.14.1
  • Updated the system requirements to reflect the above
  • Updated the changelog

Notes on reviewing/merging:

  • It may be helpful to review just the config changes (also Serial.cpp) and just the warning fixes individually. Luckily, the warning fixes are all very small; there's just a lot of them.
    • You can also review them commit-by-commit, which could make it easier to review some smaller changes separate from the ones which just add const in front of everything.
  • In order to pass the merge requirments, the required macos-11 "Test Sources" check should be replaced with the new macos-14 variant.

omichel
omichel previously approved these changes Jul 21, 2024
View reviewed changes
Copy link
Member

@omichel omichel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good job! Thank you!
I could review all the file changes as per your instructions and everything looks very good to me.
I like your approach of adding some cppcheck-suppress comments instead of trying to fix the problem when it was not super obvious to fix as it minimizes the risk of introducing some bugs, while still highlighting possible improvements.
I just replaced the mandatory macos-11 "Test Sources" check with the new macos-14 one.
I just have one minor suggestion, but you can ignore it if you prefer.

projects/robots/gctronic/e-puck/plugins/remote_controls/e-puck_wifi/Wrapper.cpp Outdated Show resolved Hide resolved
@CoolSpy3 @omichel
remove redundant if-condition
69671e0 
Co-authored-by: Olivier Michel <Olivier.Michel@cyberbotics.com>
@CoolSpy3 CoolSpy3 merged commit ac8a683 into cyberbotics:master Jul 21, 2024
21 of 22 checks passed
@CoolSpy3 CoolSpy3 mentioned this pull request Jul 29, 2024
Merged
@CoolSpy3 CoolSpy3 deleted the update-macos-runner branch July 29, 2024 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omichel omichel omichel approved these changes

Assignees
No one assigned
Labels
test distribution Start the distribution test test sources Start the sources test on all platforms test suite issue Test failure in the test suite CI test suite Start the test suite test webots build Start the build tests
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@CoolSpy3 @omichel